backend/apps/common/management/commands/load_data.py (1)

28-32: _options is accessed — drop the underscore prefix.

The leading underscore conventionally signals an unused parameter, but _options["fixture_path"] is referenced on Line 32. This is misleading to readers and linters.

Proposed fix

-    def handle(self, *_args, **_options) -> None:
+    def handle(self, *_args, **options) -> None:
         """Load data into the OWASP Nest application."""
-        with index.disable_indexing(), transaction.atomic():
-            # Run loaddata
-            call_command("loaddata", _options["fixture_path"], "-v", "3")
+        with index.disable_indexing(), transaction.atomic():
+            call_command("loaddata", options["fixture_path"], "-v", "3")

backend/tests/apps/common/management/commands/load_data_test.py (1)

23-24: Consider using MagicMock(spec=contextlib.AbstractContextManager) for mock_atomic.

Manually wiring __enter__ and __exit__ on the mock is verbose. Using MagicMock() as a context manager already works in modern unittest.mock — MagicMock supports the context-manager protocol out of the box, so these lines can simply be removed.

Also applies to: 53-54

infrastructure/staging/terraform.tfbackend.example (1)

1-3: Consider adding key and encrypt fields to the backend example.

Without a key field, Terraform defaults the state path to terraform.tfstate at the bucket root, which will collide if multiple environments or stacks share the same bucket. Adding encrypt = true is also standard practice for S3 backends.

Suggested additions

 bucket         = "${STATE_BUCKET_NAME}"
 dynamodb_table = "nest-terraform-state-lock"
+encrypt        = true
+key            = "staging/terraform.tfstate"
 region         = "us-east-2"

docker/backend/Dockerfile (1)

52-54: Minor: apk update writes an index cache that --no-cache on add won't clean up.

Consider either using --no-cache consistently (which skips the index fetch) or adding && rm -rf /var/cache/apk/* at the end. The current form works but leaves a few hundred KB of stale index data in the final image.

Suggested fix

-RUN apk update && apk --no-cache add make && \
+RUN apk --no-cache add make && \
     addgroup -S -g ${OWASP_GID} owasp && \
     adduser -S -h /home/owasp -u ${OWASP_UID} -G owasp owasp

docker-compose/local/compose.yaml (1)

144-150: Consider removing the -zappa suffix before merging to main.

Per learnings, the -zappa suffix was introduced on the feature branch to prevent volume clashes during parallel development. Once this merges to main, the isolation rationale no longer applies, and the suffix becomes a permanent naming oddity. Reverting to the original volume names (e.g., backend-venv, cache-data, db-data) would keep naming clean on the main branch.

Note: developers will need to recreate volumes either way after the merge, so there's no data-loss concern specific to renaming.

backend/wsgi.py (1)

14-25: populate_environ_from_ssm — no error handling around SSM calls.

If SSM is unreachable or the IAM role lacks permissions, the unhandled ClientError / BotoCoreError will crash the Lambda cold start with no clear diagnostic message. For Lambda, fail-fast is reasonable, but a targeted except with a clear log message would aid debugging.

This is a minor observation since fail-fast is a valid strategy here — just noting it could improve operational debuggability.

infrastructure/README.md (1)

97-100: Step 3 numbering: "Populate Secrets" could clarify which secrets need manual entry.

The instruction to populate DJANGO_* secrets with to-be-set-in-aws-console values is helpful. Consider listing the specific parameter names (or referencing the Terraform module that creates them) so operators don't have to hunt through the Parameter Store for which ones need attention.

infrastructure/modules/storage/modules/s3-bucket/main.tf (1)

20-41: Minor: Use "*" instead of { AWS = "*" } for the Principal in deny policies.

AWS documentation recommends using Principal = "*" (a plain string) rather than { AWS = "*" } for bucket policies that deny access. While functionally equivalent in deny statements, the canonical form is cleaner and avoids potential confusion.

Suggested diff

     Statement = [{
       Sid       = "HTTPSOnly"
       Effect    = "Deny"
-      Principal = { AWS = "*" }
+      Principal = "*"
       Action    = "s3:*"

infrastructure/modules/cache/main.tf (1)

44-50: Unnecessary count = 1 on random_password.

The count = 1 serves no purpose here since there's no conditional — it always creates exactly one resource. Removing it simplifies the code and eliminates the need for [0] indexing in local.redis_auth_token.

Suggested diff

 resource "random_password" "redis_auth_token" {
-  count  = 1
   length = 32
   # Redis auth token has specific requirements for special characters.
   override_special = "!$^-"
   special          = true
 }

Then update the local:

 locals {
   parameter_group_name = "default.redis${local.redis_major_version}"
-  redis_auth_token     = random_password.redis_auth_token[0].result
+  redis_auth_token     = random_password.redis_auth_token.result
   redis_major_version  = split(".", var.redis_engine_version)[0]
 }

infrastructure/modules/storage/modules/s3-bucket/outputs.tf (1)

6-9: Consider exposing specific attributes instead of the entire resource object.

Outputting the full aws_s3_bucket.this resource object couples consumers to the internal resource schema. If consumers only need id, arn, bucket, etc., exposing those individually is more intentional and resilient to provider changes.

That said, for an internal module this is acceptable if it's convenient for your use case.

infrastructure/modules/networking/modules/nacl/variables.tf (1)

1-4: Inconsistent: common_tags lacks a default here but has default = {} in sibling modules.

Other modules (e.g., security/variables.tf, s3-bucket/variables.tf) define default = {} for common_tags. Missing the default here means callers must always explicitly pass it, even if empty. Consider adding default = {} for consistency, or deliberately omit it if tags are required for this module.

infrastructure/modules/ecs/variables.tf (1)

44-48: Consider whether "latest" is a safe default for image_tag.

Defaulting to "latest" can cause non-deterministic deployments — Terraform won't detect image changes since the tag string doesn't change, and rollbacks become harder. Consider requiring this variable explicitly (no default) to force callers to pin a specific tag.

Proposed change

 variable "image_tag" {
   description = "The Docker image tag to use for ECS tasks."
   type        = string
-  default     = "latest"
 }

infrastructure/modules/database/main.tf (1)

29-34: Some special characters in override_special may cause issues in connection strings.

Characters like #, %, and & can break database connection URIs if not properly URL-encoded. Consider limiting to safer special characters.

Proposed change

 resource "random_password" "db_password" {
   count  = local.generate_db_password ? 1 : 0
   length = 32
-  # Avoid special characters that might cause issues
-  override_special = "!#$%&*()-_=+[]{}<>:?"
+  # Use only URL-safe special characters to avoid connection string issues
+  override_special = "!_-"
   special          = true
 }

infrastructure/modules/networking/.terraform.lock.hcl (1)

1-25: Lockfile is consistent with the project's exact-pinning convention.

The latest AWS provider releases go up to 6.31.0, and there's also a 6.22.1 patch release. While the team deliberately uses exact version pinning for reproducibility, consider planning a coordinated version bump across all modules when convenient — you're 9 minor versions behind. Based on learnings, the team prefers exact version pinning for maximum reproducibility.

infrastructure/modules/kms/tests/kms.tftest.hcl (2)

3-8: Policy tests validate mock data, not actual module logic.

The override_data block on Line 3-8 replaces data.aws_iam_policy_document.key_policy with a hardcoded JSON string. The test_cloudwatch_logs_policy_statement_exists and test_iam_root_policy_statement_exists runs then regex-match against this same hardcoded string. These tests will always pass regardless of what policy statements the module actually constructs, so they don't validate the real KMS key policy logic.

Consider removing the override_data block or, if it's needed for plan-mode execution, acknowledging in comments that these two tests are smoke-checking the test harness rather than the policy content.

Also applies to: 25-32, 56-63

---

74-81: Replace customer_master_key_spec with key_spec.

The customer_master_key_spec attribute is deprecated in the AWS API and Terraform provider; use key_spec instead. Both behave identically and accept the same values, but AWS recommends transitioning to key_spec going forward.

Suggested fix

   assert {
-    condition     = aws_kms_key.main.customer_master_key_spec == "SYMMETRIC_DEFAULT"
+    condition     = aws_kms_key.main.key_spec == "SYMMETRIC_DEFAULT"
     error_message = "KMS key must use SYMMETRIC_DEFAULT key spec."
   }

infrastructure/modules/parameters/tests/parameters.tftest.hcl (1)

23-421: Consider consolidating path-format and type assertions into single run blocks per parameter.

Every parameter currently uses two separate run blocks (one for path format, one for type). Since a run block supports multiple assert blocks, you could combine each pair into one run, cutting the number of test runs roughly in half and improving readability without losing any coverage. This mirrors how test_iam_role_policy_attachments_link in the frontend test file uses multiple asserts in one run.

Example consolidation for one parameter

-run "test_django_algolia_application_id_path_format" {
-  command = plan
-  assert {
-    condition     = aws_ssm_parameter.django_algolia_application_id.name == "/${var.project_name}/${var.environment}/DJANGO_ALGOLIA_APPLICATION_ID"
-    error_message = "DJANGO_ALGOLIA_APPLICATION_ID must follow path: /{project}/{environment}/DJANGO_ALGOLIA_APPLICATION_ID."
-  }
-}
-
-run "test_django_algolia_application_id_is_secure_string" {
-  command = plan
-  assert {
-    condition     = aws_ssm_parameter.django_algolia_application_id.type == "SecureString"
-    error_message = "DJANGO_ALGOLIA_APPLICATION_ID must be stored as SecureString."
-  }
-}
+run "test_django_algolia_application_id" {
+  command = plan
+  assert {
+    condition     = aws_ssm_parameter.django_algolia_application_id.name == "/${var.project_name}/${var.environment}/DJANGO_ALGOLIA_APPLICATION_ID"
+    error_message = "DJANGO_ALGOLIA_APPLICATION_ID must follow path: /{project}/{environment}/DJANGO_ALGOLIA_APPLICATION_ID."
+  }
+  assert {
+    condition     = aws_ssm_parameter.django_algolia_application_id.type == "SecureString"
+    error_message = "DJANGO_ALGOLIA_APPLICATION_ID must be stored as SecureString."
+  }
+}

infrastructure/modules/database/outputs.tf (1)

6-9: Description is misleading when create_rds_proxy is false.

When var.create_rds_proxy is false, this output returns the RDS instance address, not a proxy endpoint. Consider updating the description to reflect the conditional behavior.

Suggested fix

 output "db_proxy_endpoint" {
-  description = "The endpoint of the RDS proxy."
+  description = "The database connection endpoint (RDS Proxy when enabled, otherwise RDS instance address)."
   value       = var.create_rds_proxy ? aws_db_proxy.main[0].endpoint : aws_db_instance.main.address
 }

infrastructure/modules/cache/outputs.tf (1)

1-4: Minor grammar nit in description.

Suggested fix

 output "redis_password_arn" {
-  description = "The SSM Parameter ARN of password of Redis."
+  description = "The SSM Parameter ARN of the Redis password."
   value       = aws_ssm_parameter.django_redis_password.arn
 }

infrastructure/modules/ecs/outputs.tf (1)

11-14: Minor description inconsistency: "ECS tasks cluster" vs "ECS cluster".

The frontend module's equivalent output (Line 12 of frontend/outputs.tf) uses "The name of the ECS cluster." while this says "The name of the ECS tasks cluster." Consider aligning the wording for consistency across modules.

Suggested fix

 output "ecs_cluster_name" {
-  description = "The name of the ECS tasks cluster."
+  description = "The name of the ECS cluster."
   value       = aws_ecs_cluster.main.name
 }

infrastructure/modules/security/main.tf (1)

48-56: Frontend port 3000 is hardcoded in security group rules.

The ALB module receives frontend_port as a variable (set to 3000 in staging), but the security module hardcodes port 3000 in both alb_to_frontend egress (Line 50/54) and frontend_from_alb ingress (Line 99/103). If frontend_port ever changes, these rules would silently become stale.

Consider passing frontend_port as a variable to the security module to keep these in sync.

♻️ Suggested approach

Add a frontend_port variable to the security module and use it in both rules:

 resource "aws_security_group_rule" "alb_to_frontend" {
   description              = "Allow traffic to frontend ECS tasks"
-  from_port                = 3000
+  from_port                = var.frontend_port
   protocol                 = "tcp"
   security_group_id        = aws_security_group.alb.id
   source_security_group_id = aws_security_group.frontend.id
-  to_port                  = 3000
+  to_port                  = var.frontend_port
   type                     = "egress"
 }

Apply the same to frontend_from_alb.

Also applies to: 97-105

infrastructure/modules/networking/tests/networking.tftest.hcl (1)

244-263: Good positive and negative testing for VPC endpoint module creation.

The default-disabled test ensures no unnecessary resources are created, while the create_vpc_s3_endpoint = true override validates that enabling any single endpoint flag triggers module creation. Consider adding a test that enables multiple endpoints simultaneously to verify there's still only one module instance.

infrastructure/backend/main.tf (1)

195-211: Consider using KMS encryption for the state bucket instead of AES256.

The DynamoDB lock table uses the customer-managed KMS key from module.kms (line 93), but both S3 buckets fall back to AES256 (SSE-S3). Since the state bucket holds sensitive Terraform state (potentially containing secrets and resource identifiers), using the same KMS key would provide consistent encryption posture and key management controls.

♻️ Proposed fix for state bucket encryption

 resource "aws_s3_bucket_server_side_encryption_configuration" "state" {
   bucket = aws_s3_bucket.state.id
   rule {
     apply_server_side_encryption_by_default {
-      sse_algorithm = "AES256"
+      sse_algorithm     = "aws:kms"
+      kms_master_key_id = module.kms.key_arn
     }
+    bucket_key_enabled = true
   }
 }

infrastructure/modules/ecs/modules/task/main.tf (1)

37-37: Consider parameterizing the container name.

The container name is hardcoded to "backend", but this task module is reused for multiple task types (sync_data, migrate, index_data, etc.). Using var.task_name (or a dedicated variable) would make the module more self-describing and easier to target for container overrides in RunTask API calls.

♻️ Suggested change

-      name = "backend"
+      name = var.task_name

infrastructure/modules/networking/modules/nacl/main.tf (1)

38-46: Private ephemeral inbound from 0.0.0.0/0 is intentionally broad — confirm this is expected.

Rule 100 allows inbound TCP 1024–65535 from 0.0.0.0/0 on the private NACL. This is standard for accepting return traffic through a NAT Gateway, but it also admits unsolicited traffic on ephemeral ports from any source that somehow reaches the private subnet. Since NACLs are stateless, this is the typical trade-off. Just flagging for awareness — security groups provide the stateful second layer.

infrastructure/modules/alb/outputs.tf (1)

31-35: depends_on on output block — valid but worth a brief inline comment.

The depends_on = [aws_lb_listener.https] ensures the HTTPS listener exists before downstream modules consume the target group ARN. This is a correct ordering safeguard. Consider adding a brief comment explaining why this dependency exists, since depends_on on outputs is uncommon and might surprise future maintainers.

infrastructure/modules/frontend/variables.tf (1)

51-55: image_tag defaults to "latest" — acceptable for testing but worth noting.

For a testing environment this is fine. If this module is ever promoted to production, pinning to a specific tag would be important to ensure reproducible deployments.

infrastructure/modules/alb/variables.tf (1)

45-49: Minor naming inconsistency: log_retention_days vs log_retention_in_days in other modules.

The networking module and frontend module use log_retention_in_days, while this ALB module uses log_retention_days. Consider aligning the naming for consistency across the infrastructure modules.

infrastructure/modules/networking/modules/vpc-endpoint/main.tf (1)

37-122: Interface endpoints are consistently structured — well-factored repetitive resources.

All five interface endpoints follow the same pattern (private DNS, shared SG, private subnets, consistent tagging). If the number of endpoints grows further, you could consider using for_each over a map of service names to reduce repetition, but at five endpoints the current approach is clear and maintainable.

infrastructure/modules/frontend/main.tf (1)

109-136: Consider adding deployment_circuit_breaker to the ECS service.

Without a circuit breaker, a bad deployment can leave the service in a degraded state with ECS continuously trying to place failing tasks. Enabling it with rollback provides automatic recovery.

♻️ Suggested addition

 resource "aws_ecs_service" "frontend" {
   cluster                           = aws_ecs_cluster.frontend.id
   desired_count                     = var.desired_count
   health_check_grace_period_seconds = 60
   name                              = "${var.project_name}-${var.environment}-frontend-service"
   ...
   task_definition = aws_ecs_task_definition.frontend.arn
 
+  deployment_circuit_breaker {
+    enable   = true
+    rollback = true
+  }
+
   capacity_provider_strategy {

infrastructure/modules/alb/main.tf (2)

83-101: enable_deletion_protection is hardcoded to false.

If this module is intended for reuse across staging and production, consider parameterizing this. Accidental deletion of a production ALB could cause an outage.

♻️ Suggested change

-  enable_deletion_protection = false
+  enable_deletion_protection = var.enable_deletion_protection

Add to variables.tf:

variable "enable_deletion_protection" {
  description = "Whether to enable deletion protection on the ALB."
  type        = bool
  default     = false
}

---

134-149: Listener rule priority uses each.key which is a string, not an integer.

The for_each map uses string keys (from for idx, chunk). While Terraform will implicitly convert the string index to a number in 100 + each.key, this relies on implicit type coercion. The expression works correctly because idx produces numeric strings ("0", "1", "2"), but it would be clearer to use tonumber(each.key).

♻️ Minor clarification

-  priority     = 100 + each.key
+  priority     = 100 + tonumber(each.key)

.github/workflows/run-ci-cd.yaml (3)

537-545: Long-lived AWS access keys used instead of OIDC.

The workflow already uses OIDC (id-token: write) for Codecov in other jobs. Using OIDC with aws-actions/configure-aws-credentials for AWS access would eliminate the need for long-lived AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY secrets, reducing the blast radius if credentials are compromised. This applies to all three jobs that configure AWS credentials (build-staging-images, plan-staging-nest, deploy-staging-nest).

---

818-826: Plan artifact may become stale between plan and apply jobs.

If infrastructure state changes between the plan-staging-nest and deploy-staging-nest jobs (e.g., manual changes, concurrent runs), terraform apply tfplan will fail because the saved plan's state serial won't match. This is actually the correct behavior — Terraform will refuse to apply a stale plan. Just be aware that this can cause deploy failures requiring a re-run of the pipeline.

Also, note that -auto-approve is redundant when applying a saved plan file.

---

857-870: Zappa deployment depends on Terraform outputs but has no rollback if zappa update fails.

If zappa update staging (line 870) fails mid-deploy, the Terraform infrastructure will already be applied but the Lambda function may be in an inconsistent state. Consider adding error handling or a rollback step, or at minimum ensuring zappa update is idempotent and safe to re-run.

infrastructure/modules/networking/main.tf (1)

60-67: Single NAT gateway — acceptable for staging, worth noting for production.

The NAT gateway is pinned to aws_subnet.public[0], meaning all private-subnet egress routes through a single AZ. This is cost-effective for staging but creates a single point of failure. If this module is reused for production, consider provisioning one NAT gateway per AZ for high availability.

infrastructure/modules/parameters/main.tf (1)

16-26: SecureString parameters use AWS-managed KMS — track CMK migration.

Checkov flags CKV_AWS_337 on all SecureString parameters because they rely on the default AWS-managed aws/ssm key rather than a customer-managed KMS CMK. Based on learnings, KMS CMK encryption is planned after S3 state management is completed. Leaving this as a tracking note — no action needed now, but ensure this is on the roadmap.

Also applies to: 109-119, 149-159, 162-172, 174-184, 218-228

infrastructure/staging/variables.tf (1)

1-5: No validation that AZ count matches subnet CIDR counts.

The networking module indexes into availability_zones, public_subnet_cidrs, and private_subnet_cidrs by count.index (e.g., var.availability_zones[count.index]). If the lists have mismatched lengths, Terraform will fail with an index-out-of-range error at plan time. Consider adding a cross-variable validation.

🛡️ Example validation (add to any one of the three variables)

variable "availability_zones" {
  description = "A list of availability zones for the VPC."
  type        = list(string)
  default     = ["us-east-2a", "us-east-2b", "us-east-2c"]

  validation {
    condition     = length(var.availability_zones) > 0
    error_message = "At least one availability zone must be specified."
  }
}

Note: Terraform variable-level validations cannot cross-reference other variables. The best place for a cross-variable check is a check block or a precondition in the module call in main.tf:

locals {
  az_count = length(var.availability_zones)
}

check "subnet_az_count_match" {
  assert {
    condition     = length(var.public_subnet_cidrs) == local.az_count && length(var.private_subnet_cidrs) == local.az_count
    error_message = "The number of public/private subnet CIDRs must match the number of availability zones."
  }
}

Also applies to: 201-231

infrastructure/README.md (3)

61-65: Prefer cp over cat > for copying files.

The command cat terraform.tfvars.example > terraform.tfvars works but is less idiomatic than cp terraform.tfvars.example terraform.tfvars. The standard copy command is clearer and more direct.

📝 Suggested simplification

-  - Copy the contents from the example file:
-
-    ```bash
-    cat terraform.tfvars.example > terraform.tfvars
-    ```
+  - Copy the example file:
+
+    ```bash
+    cp terraform.tfvars.example terraform.tfvars
+    ```

---

73-77: Prefer cp over cat > for copying files.

Same as the previous comment—using cp is more idiomatic than cat > for file copying.

📝 Suggested simplification

-  - Copy the contents from the example file:
-
-    ```bash
-    cat terraform.tfbackend.example > terraform.tfbackend
-    ```
+  - Copy the example file:
+
+    ```bash
+    cp terraform.tfbackend.example terraform.tfbackend
+    ```

---

132-136: Prefer cp over cat > for copying files.

Same pattern as earlier—cp is more idiomatic than cat > for file copying.

📝 Suggested simplification

-  - Copy the contents from the template file into your new local environment file:
-
-    ```bash
-    cat zappa_settings.template.json > zappa_settings.json
-    ```
+  - Copy the template file:
+
+    ```bash
+    cp zappa_settings.template.json zappa_settings.json
+    ```

.github/workflows/run-ci-cd.yaml (1)

621-626: Consider using OIDC for AWS authentication instead of static access keys.

Both plan-staging-nest and deploy-staging-nest use long-lived AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY static credentials. GitHub Actions supports OIDC federation with AWS via role-to-assume, which eliminates the need for stored secrets and provides short-lived credentials. This is a security best practice, especially for a public repository.

Example OIDC configuration

       - name: Configure AWS credentials
         uses: aws-actions/configure-aws-credentials@61815dcd50bd041e203e49132bacad1fd04d2708
         with:
-          aws-access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }}
           aws-region: ${{ vars.AWS_REGION }}
-          aws-secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
+          role-to-assume: ${{ secrets.AWS_ROLE_ARN }}

This requires setting up an IAM OIDC provider and role in AWS, and adding id-token: write to the job's permissions.

Also applies to: 718-723

infrastructure/backend/main.tf (2)

23-35: Consider adding an HTTPS-only policy for the logs bucket as well.

The state bucket correctly enforces HTTPS-only access via state_https_only, but the logs bucket has no equivalent deny policy. IAM principals with access to the logs bucket could potentially interact with it over plain HTTP. For consistency and defense-in-depth, apply the same SecureTransport deny condition to the logs bucket.

♻️ Suggested addition

+data "aws_iam_policy_document" "logs_https_only" {
+  policy_id = "ForceHTTPS"
+
+  statement {
+    actions = ["s3:*"]
+    sid     = "HTTPSOnly"
+    effect  = "Deny"
+
+    condition {
+      test     = "Bool"
+      values   = ["false"]
+      variable = "aws:SecureTransport"
+    }
+    principals {
+      identifiers = ["*"]
+      type        = "AWS"
+    }
+    resources = [
+      aws_s3_bucket.logs.arn,
+      "${aws_s3_bucket.logs.arn}/*",
+    ]
+  }
+}

Then combine both statements in the logs bucket policy (or use source_policy_documents / override_policy_documents to merge them):

 resource "aws_s3_bucket_policy" "logs" {
   bucket = aws_s3_bucket.logs.id
-  policy = data.aws_iam_policy_document.logs.json
+  policy = data.aws_iam_policy_document.logs_combined.json
 }

---

195-213: AES256 encryption is acknowledged via trivy:ignore — consider upgrading the state bucket to KMS-CMK.

The Trivy suppressions indicate this is intentional. AES256 for the logs bucket is reasonable since S3 server access logging can have compatibility issues with KMS-encrypted target buckets. However, the state bucket holds sensitive infrastructure state and would benefit from CMK encryption (matching the DynamoDB table's approach), with key rotation and audit trail via CloudTrail. This could be deferred if the KMS module is still being stabilized.

infrastructure/modules/ecs/main.tf (1)

14-28: ECS tasks cluster is missing containerInsights.

The frontend module's cluster (Line 64-67 of frontend/main.tf) explicitly enables containerInsights, but this tasks cluster does not. Without it, you lose ECS-level CPU/memory/task metrics in CloudWatch, making it harder to troubleshoot scheduled task failures or resource pressure.

♻️ Proposed fix to enable Container Insights

 resource "aws_ecs_cluster" "main" {
   name = "${var.project_name}-${var.environment}-tasks-cluster"
   tags = var.common_tags
+
+  setting {
+    name  = "containerInsights"
+    value = "enabled"
+  }
 }

infrastructure/modules/frontend/main.tf (1)

109-136: Consider enabling a deployment circuit breaker for the frontend ECS service.

The aws_ecs_service resource does not configure deployment_circuit_breaker. Without it, a broken image deployment can leave the service stuck in a rollout loop with failing tasks draining capacity. Enabling the circuit breaker with rollback provides a safety net.

♻️ Suggested addition

 resource "aws_ecs_service" "frontend" {
   cluster                           = aws_ecs_cluster.frontend.id
   desired_count                     = var.desired_count
   health_check_grace_period_seconds = 60
   name                              = "${var.project_name}-${var.environment}-frontend-service"
   ...
   task_definition = aws_ecs_task_definition.frontend.arn
 
+  deployment_circuit_breaker {
+    enable   = true
+    rollback = true
+  }
+
   capacity_provider_strategy {

infrastructure/modules/networking/main.tf (2)

60-67: Single NAT gateway creates a single point of failure across AZs.

All private subnets route through one NAT gateway in public[0]. If that AZ becomes unavailable, all private subnet egress (ECR pulls, DB connections via public endpoints, external API calls) will fail. For a staging environment this is a reasonable cost trade-off, but for production consider deploying one NAT gateway per AZ.

---

22-43: Subnet creation assumes matching array lengths for AZs and CIDRs.

count = length(var.public_subnet_cidrs) (Line 25) indexes into var.availability_zones[count.index] (Line 23). If the AZ list is shorter than the CIDR list, this will fail at plan time with an index-out-of-range error. Consider adding a validation block in the variables file to ensure length(var.availability_zones) >= length(var.public_subnet_cidrs) and similarly for private subnets.

#!/bin/bash
# Check if there's any validation in the networking module's variables.tf
fd -p 'modules/networking/variables.tf' --exec grep -A 10 'validation' {} \;

infrastructure/modules/alb/main.tf (2)

135-150: Listener rule priority relies on implicit string-to-number conversion.

each.key from the for_each map is a string (e.g., "0", "1", "2"), so 100 + each.key relies on Terraform's implicit type coercion. While this works, an explicit tonumber() call would improve clarity.

♻️ Explicit type conversion

-  priority     = 100 + each.key
+  priority     = 100 + tonumber(each.key)

---

83-102: ALB has enable_deletion_protection = false.

This is fine for staging/dev, but for production the ALB should have deletion protection enabled to prevent accidental terraform destroy. Consider making this a variable so it can be toggled per environment.